Sublist Shared Cache Improvements #726
Conversation
Signed-off-by: Derek Collison <[email protected]>
Signed-off-by: Derek Collison <[email protected]>
server/sublist.go
Outdated
| r := v.(*SublistResult) | ||
| if matchLiteral(key, subject) { | ||
| s.cache.Store(key, r.addSubToResult(sub)) | ||
| return false |
There was a problem hiding this comment.
Return false will stop the iteration. That's not what the original code was doing.
There was a problem hiding this comment.
Correct, will fix. I will add test too.
There was a problem hiding this comment.
I added separate test, but could add this into the larger cache test if you think that is better.
There was a problem hiding this comment.
I moved it into larger cache test and also double checked that it failed without the change.
| checkBool(subjectIsLiteral("foo.*.>"), false, t) | ||
| checkBool(subjectIsLiteral("foo.*.bar"), false, t) | ||
| checkBool(subjectIsLiteral("foo.bar.>"), false, t) | ||
| } |
There was a problem hiding this comment.
I think we had changes in the not so distant past that made say foo*.bar a literal. You may want to add a test that we indeed consider * and > that are not token of their own not wildcards.
Signed-off-by: Derek Collison <[email protected]>
Signed-off-by: Derek Collison <[email protected]>
Signed-off-by: Derek Collison <[email protected]>
|
Let me make sure we are green then I will merge. |
There was a problem hiding this comment.
Thanks for addressing this issue.
I think this should clear the contention issues I expressed in #710 for the most part since the write lock is no longer held on matching and the sweeping is async.
However, I still feel both the server's "client" cache and sublist cache are inefficient in this use case and I'd rather disable them entirely.
I will run my experiment again and compare with the no cache version I have and let you know. I will also run the benchmark suite, the contention benchmarks are a good addition :)
I think there may be a race in the cache handling.
| delete(s.cache, k) | ||
| break | ||
| } | ||
| s.cache.Store(subject, result) |
There was a problem hiding this comment.
I think this introduces a race. For example, if a client subscribes between the unlock and the call to s.cache.Store it will be shadowed and won't be delivered messages for this subscription until the cache entry gets cleared.
Also for queue subscriptions a client could have removed a subscription (but still be connected) and be delivered messages while no longer processing it and would "steal" it from other valid subscriptions of the same queue.
The read lock should be held while calling s.cache.Store to ensure that the sublist won't be modified concurrently.
There may be similar issues with Load, although I don't see one at the moment holding the read lock is probably a good idea too.
There was a problem hiding this comment.
exactly why I disliked that Go added sync.Map. It "introduces" these kind of issues.
There was a problem hiding this comment.
Same class of problems may happen with atomic operations but they are still useful. Concurrency is hard to reason about...
There was a problem hiding this comment.
Let me take a closer look. Let me know if your production issue resolves with this fix though.
There was a problem hiding this comment.
ok #729 should handle. Thanks for pointing this out, you were correct.
Sublists have a caching layer that is shared. Most of the extreme performance in NATS comes from the client L1 cache of the sublist which has no contention. This works well as long as the sublist has a fairly low rate of change. When highly concurrent reads to the sublist's cache were present performance suffered.
Also #710 showed a case where the shared cache made things worse.
This PR alleviates some of those issues by being smarter when having a literal subject to add or remove from the shared cache.
It also uses Go 1.9 sync.Map which performs better under highly concurrent conditions with many cores.
/cc @nats-io/core